feat: Create collection Modal [FC-0062] #1259
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @ChrisChV! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
ChrisChV
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1259 +/- ##
==========================================
+ Coverage 92.31% 92.32% +0.01%
==========================================
Files 1013 1016 +3
Lines 18733 18803 +70
Branches 3947 4017 +70
==========================================
+ Hits 17293 17360 +67
Misses 1374 1374
- Partials 66 69 +3 ☔ View full report in Codecov by Sentry. |
| key={`add-content-${blockType}`} | ||
| variant="outline-primary" | ||
| disabled={disabled} | ||
| className="m-2 rounded-0" |
There was a problem hiding this comment.
This is more of a general comment (not related to this PR).
I don't think we should set rounded-0 here (and I don't think it should be that way in the design).
@bradenmacdonald, should we ask the design team to use the default Paragon theme? It is hard to be consistent in manually implementing these overrides, and if we hardcode these styles, we prevent the theme from being applied properly in my opinion.
There was a problem hiding this comment.
Yeah - in general, please just use the Paragon styles and don't override them unless there's a very specific reason required for the design. That way the components will be consistent, and it also makes development faster. This is also an MVP so we're not generally aiming for pixel-perfect UX design.
@sdaitzman @lizc577 can I get your thoughts - do you have a Paragon theme for Figma now?
There was a problem hiding this comment.
Which border radius is this about? Edit: I looked through the TypeScript, if I'm correct this is about the Add Content button: no need to override the Paragon theme border radius.
Most components and workflows in Figma are using Paragon Figma components (a few screens haven't been updated, and there are occasional overrides to fix UX issues in the original Paragon components).
The Paragon Figma library is based on the 2U/EdX theme, so we don't expect the implemented components to match exactly. The UX/UI and Paragon WGs have been discussing building an Open EdX Paragon Figma library, but only a small fraction exists right now.
There was a problem hiding this comment.
Thanks for the clarification. Updated here: b0a47cc
There was a problem hiding this comment.
Thank you for your input here @sdaitzman!
We have some buttons for which we overrode the radius to match the design and some for which we did not.
From now on, I will not override the Paragon styles.
If you catch some inconsistencies in the UX/UI validation, please let us know!!
| } = React.useContext(LibraryContext); | ||
| const { showToast } = React.useContext(ToastContext); | ||
|
|
||
| const [collectionName, setCollectionName] = React.useState<string | null>(null); |
There was a problem hiding this comment.
We should not use null as a value on an input. It should be '' for a controlled component. This generates some warnings in the console.
| const [collectionName, setCollectionName] = React.useState<string | null>(null); | |
| const [collectionName, setCollectionName] = React.useState<string>(''); |
Also, I think it would be better to use Formik + Yup for form validation and state management instead of doing it manually. We have a recent example here: https://github.com/openedx/frontend-app-course-authoring/blob/master/src/library-authoring/create-library/CreateLibrary.tsx
There was a problem hiding this comment.
LGTM 👍
Thank you for your work, @ChrisChV!
It worked flawlessly.
I added some optional suggestions about using the formik, but feel free to push back if you want to!
- I tested this using the instructions from the PR
- I read through the code
- I checked for accessibility issues
- Includes documentation
src/library-authoring/create-collection/CreateCollectionModal.tsx
Outdated
Show resolved
Hide resolved
| const [isCreatingCollection, setIsCreatingCollection] = React.useState<boolean>(false); | ||
|
|
||
| const handleOnClose = React.useCallback(() => { | ||
| closeCreateCollectionModal(); | ||
| setIsCreatingCollection(false); | ||
| }, []); | ||
|
|
There was a problem hiding this comment.
Then this can go
| const [isCreatingCollection, setIsCreatingCollection] = React.useState<boolean>(false); | |
| const handleOnClose = React.useCallback(() => { | |
| closeCreateCollectionModal(); | |
| setIsCreatingCollection(false); | |
| }, []); |
src/library-authoring/create-collection/CreateCollectionModal.tsx
Outdated
Show resolved
Hide resolved
src/library-authoring/create-collection/CreateCollectionModal.tsx
Outdated
Show resolved
Hide resolved
src/library-authoring/create-collection/CreateCollectionModal.tsx
Outdated
Show resolved
Hide resolved
| )} | ||
| value={formikProps.values.title} | ||
| placeholder={intl.formatMessage(messages.createCollectionModalNamePlaceholder)} | ||
| help="" |
There was a problem hiding this comment.
Nit
| help="" |
There was a problem hiding this comment.
I don't understand.. help has a defaultProp -- why isn't it ok to remove it?
But if it must be included, then just send an empty element so we don't have to change the expected types:
| help="" | |
| help=<></> |
src/library-authoring/create-collection/CreateCollectionModal.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Hi @ChrisChV , this is working well, but I have some minor comments. Other than removing the unused message, none of my comments need to block merging, though, so:
👍
- I tested this using the branch from Store Collection metadata + component count in meilisearch open-craft/edx-platform#684 on the backend and the PR test instructions.
- I read through the code
- I checked for accessibility issues by using my keyboard to navigate.
-
Includes documentationN/A - User-facing strings are extracted for translation
src/generic/FormikControl.jsx
Outdated
| label: PropTypes.oneOfType([PropTypes.string, PropTypes.element]), | ||
| help: PropTypes.oneOfType([PropTypes.string, PropTypes.element]), |
There was a problem hiding this comment.
label is still always a PropTypes.element.. and help would be if you just wrapped it in a <Form.Text> element.
So rather than making this change, can we please just call it with elements instead of strings?
| )} | ||
| value={formikProps.values.title} | ||
| placeholder={intl.formatMessage(messages.createCollectionModalNamePlaceholder)} | ||
| help="" |
There was a problem hiding this comment.
I don't understand.. help has a defaultProp -- why isn't it ok to remove it?
But if it must be included, then just send an empty element so we don't have to change the expected types:
| help="" | |
| help=<></> |
| )} | ||
| value={formikProps.values.description} | ||
| placeholder={intl.formatMessage(messages.createCollectionModalDescriptionPlaceholder)} | ||
| help={intl.formatMessage(messages.createCollectionModalDescriptionDetails)} |
There was a problem hiding this comment.
Please send an element instead of a string so we don't have to change the types:
| help={intl.formatMessage(messages.createCollectionModalDescriptionDetails)} | |
| help={( | |
| <Form.Text> | |
| {intl.formatMessage(messages.createCollectionModalDescriptionDetails)} | |
| </Form.Text> | |
| )} |
| createCollectionModalNameConflict: { | ||
| id: 'course-authoring.library-authoring.modals.create-collection.form.name.conflict', | ||
| defaultMessage: 'There is another collection with the same name', | ||
| description: 'Message when the Name field of the Create Collection modal form is not unique', | ||
| }, |
There was a problem hiding this comment.
This message isn't used anywhere:
| createCollectionModalNameConflict: { | |
| id: 'course-authoring.library-authoring.modals.create-collection.form.name.conflict', | |
| defaultMessage: 'There is another collection with the same name', | |
| description: 'Message when the Name field of the Create Collection modal form is not unique', | |
| }, |
|
@bradenmacdonald I need a green check from a reviewer with write access. Could you review this? |
|
@ChrisChV, sure. Just updating my devstack now. I did already notice one minor issue: when I click this "Add Collection" button, it doesn't open the "New Collection" modal - it just shows the general "Add" sidebar, which is not what I'd expect.
|
Description
Adds a functional Modal to create a library collection.
Supporting information
Testing instructions
edx-platform: Update collections crud rest api open-craft/edx-platform#683NewbuttonAdd contentsidebar.Createwith all empty. Verify the validation error.Create. Verify the success.tutor dev run cms ./manage.py cms reindex_studio --experimentaland refresh the library home page to see the newly created collection.Createand verify the validation error.